feat(tui): add compact image paste placeholders - #418
Conversation
github-actions
Bot
added
the
documentation
tt-a1i
left a comment
tt-a1i
left a comment
There was a problem hiding this comment.
Reviewed the current head 9c7f9ba. The compact placeholders and atomic deletion address a useful TUI problem, but attachment ownership still breaks across submission paths. Please address the two inline findings before merging.
Validation: the 11 image-paste/editor-layer tests pass locally, and all three required CI jobs are green. Additional minimal reproductions using Pi 0.85.1's actual InteractiveMode.flushCompactionQueue and ExtensionRunner.emitInput methods reproduce both findings. These are programmatic lifecycle reproductions, not visual TUI acceptance or live provider calls.
tt-a1i
left a comment
tt-a1i
left a comment
There was a problem hiding this comment.
Reviewed at head bc2739f.
Standards
[P1] Image ownership is inferred only from the OS temp directory, a pi-clipboard filename pattern, and statSync(). statSync follows symlinks, and the later read/delete has a check-to-use race. The extension can therefore claim and remove a matching file without runtime-proven ownership. Please use a Pi-provided attachment handle, or copy into an OpenPI-owned directory and validate identity with no-follow, bounded I/O.
[P1] The 481-line editor layer depends on several private lifecycle details at once: onSubmit wrapping, raw Alt+Enter handling, setText cleanup timing, input ordering, and compaction retry behavior. Pi 0.85.1 still exposes no stable attachment/submission seam, so upgrades can silently break ownership. This should be reduced around an explicit Pi-native attachment/submission boundary rather than duplicating InteractiveMode lifecycle assumptions.
[P2] The input handler performs unbounded synchronous readFileSync plus base64 conversion. A large clipboard file can block the TUI and amplify memory use. Add a size limit and bounded asynchronous reading after identity validation.
Spec
[P1] Successful compaction with multiple queued submissions is still unsafe. Every submission enters pending, but after normal compaction only the first queued message goes through prompt/input; later messages use steer/followUp and bypass the input transform. Their images are neither sent nor promptly cleaned, and a later identical text can consume an old FIFO submission. The existing test covers only one willRetry=true submission, not successful compaction with multiple queued submissions or repeated identical text.
The Alt+Enter cleanup race from the previous review is fixed, but attachment ownership across compaction remains incomplete. Please add the missing lifecycle evidence and resolve the current main conflict before requesting re-review.
…ge-paste # Conflicts: # README.md
Submission expands placeholders into real clipboard paths so the model receives byte-identical input to unmodified Pi and the read tool can still open the file. That moved issue openpi-dev#413's long temp paths out of the editor and into the transcript, so the noise the issue reported was relocated rather than removed. Collapse those paths back into compact placeholders at render time through registerMarkdownTransformer, the same seam Pi's built-in mermaid renderer uses. The transform is stateless and scoped to user messages, so replays, forks and reloaded sessions render identically without carrying a mapping through submission, and paths the assistant quotes are left untouched. Match the directory portion as discrete segments that exclude ':' and are lazy rather than greedy. Pasting images back to back leaves adjacent paths with no separator, and a greedy middle absorbed the next path's 'C:' or '/tmp' and rendered several images as a single placeholder.
|
Thanks for the review. I've reworked the approach rather than patching the individual findings, because the structural concern turned out to be the root of the others. What changed The previous version converted placeholders into base64 This version follows Pi's own long-paste marker pattern instead: the buffer shows a compact marker, submission expands it back to the real content. Concretely, the extension no longer reads files, no longer produces base64, and no longer deletes anything. The model receives the real clipboard path, byte-identical to unmodified Pi, and the
How that maps to your findings
The main conflict is also resolved. One thing worth flagging Expanding at submission solved the model side but relocated the problem the issue actually reported: the long temp path moved out of the editor and into the transcript. I've added a rendering-only counterpart via While testing that I found and fixed a real bug: pasting images back to back produces adjacent paths with no separator, and a greedy directory match absorbed the following path's Known limitation Transcript numbering follows the order paths appear in the message, so it can differ from the editor if images were deleted mid-draft. Making it strictly identical would require carrying a mapping through message metadata, which is outside the seams available to an extension. Since transcript numbering only distinguishes images within a single message, I've documented it rather than worked around it. Longer term, folding image paths into Pi's native paste marker registry seems like the better home for this. Happy to open an upstream issue if you agree. 24 targeted tests pass; type check, lint and format are clean. |
2 participants
问题
关联 #413。
在 TUI 中粘贴图片时，输入框会显示冗长的临时文件路径，影响提示词的阅读和编辑，多图场景下尤其明显。
价值
[Image #N]展示粘贴的图片。read工具仍可打开图片。实现方案
本次改动遵循 Pi 原生长文本粘贴（paste marker）的既有范式：编辑器缓冲区显示紧凑标记，提交时展开回真实内容。图片粘贴复用同一套思路，而不是新建一套附件所有权模型。
编辑与提交：
[Image #N]，并在当前草稿中维护占位符与图片路径的对应关系。[Image #N]。getExpandedText()展开占位符。这是 Pi 读取待提交文本的统一入口，handleFollowUp()在选择prompt()、queueCompactionMessage()或onSubmit之前都会先经过它，因此 Alt+Enter 的空闲、流式、压缩三条分支都能正确展开。渲染：
registerMarkdownTransformer在渲染时把剪贴板路径折叠回[Image #N]，这是 Pi 内置 mermaid 渲染使用的同一个公开接缝。messageType === "user"生效，助手正常引用的路径不受影响。与上一版的差异
上一版把占位符在
input事件中转换为 base64ImageContent，并在读取后删除临时文件。这引入了一套 Pi 之外的附件所有权与生命周期管理，需要依赖多个私有实现细节，也放弃了原生「路径长期有效、read工具随时可读」的行为。本版改为不接管生命周期：不读文件、不转 base64、不删除临时文件。模型收到真实路径，与原生 Pi 字节一致。相应地，
Submission队列、pending映射、input与session_compact钩子全部移除。这同时消解了上一轮 review 中关于压缩队列绕过
input导致图片丢失、无界同步读取、以及编辑器层依赖过多私有生命周期的意见。验证
getExpandedText()不修改缓冲区、歧义占位符保持折叠。C:或/tmp吞并，导致多张图片渲染成一个占位符。[Image #1][Image #2]，发送后 transcript 同样显示占位符，模型收到完整路径并能通过read工具读取。影响
改动仅影响 TUI 交互式图片粘贴，不改变发送给模型的消息内容，也不改变临时文件的生命周期。
已知限制：transcript 中的编号按路径在该条消息中的出现顺序重排。如果编辑过程中删除过图片，显示编号可能与输入时不完全一致。要做到严格一致需要把映射写入消息元数据，超出扩展可用的接缝范围，而 transcript 编号仅用于区分同一条消息内的不同图片，因此接受该行为。
后续可考虑向上游提议将图片路径纳入原生 paste marker 机制，那是更合适的归属位置。